-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Init rntr #1
base: main
Are you sure you want to change the base?
Conversation
} | ||
|
||
interface IFabricUIManagerMock extends FabricUIManager { | ||
fireEvent( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] Consider naming dispatchEvent
to match browser JS naming: https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/dispatchEvent
Afaik, fireEvent
is Testing Library specific name.
module.exports = { | ||
haste: { | ||
defaultPlatform: 'ios', | ||
platforms: ['android', 'ios', 'native'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you plan to have API to specify which platform to render as (ios, android)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a task to follow up with a way to control this
type ReactNode = { | ||
children: Array<ReactNode>, | ||
props: {...}, | ||
viewName: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the host component name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of instanceHandle
?
How memoizedProps
compare to regular props
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
host component name
You can see in the snapshot output, its RCTView or RCTText in this example. I'll have to look back at this with some custom components and see what those will come in as.
memoizedProps
I can't remember at the moment. But I think some properties weren't available as props
but were in memoizedProps
when I was working on this. Will look into it
import {act} from 'react-test-renderer'; | ||
import ReactFabric from 'react-native/Libraries/Renderer/shims/ReactFabric'; | ||
|
||
type ReactNode = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to provide toJson()
on React Node, so that users could do focussed snapshots on only a part of the element tree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will give this a try as a follow up
} | ||
} | ||
|
||
function findBy(node: ReactNode, predicate: ReactNode => boolean): ?ReactNode { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to return all nodes matching predicate, not just the first one. RNTL queries always validate the number of matching elements in the tree vs expected number informed by query variant (getBy
vs getAllBy
vs queryBy
),
}, | ||
}; | ||
|
||
type RootReactNode = $ReadOnlyArray<ReactNode>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: in what cases root node can be an array of nodes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here... this is a manual typing based on the outputs we were inspecting from Fabric. Can be improved or resused from official types probably.
} | ||
const containerTag = Math.round(Math.random() * 1000000); | ||
act(() => { | ||
ReactFabric.render(element, containerTag, () => {}, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to configure renderer to mimic RN renderer behavior of throwing error on passing text children to non-Text
elements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it seems better if that comes for free as part of using RN/Fabric rather than re-implementing one-off errors in the test renderer (if possible). I'm not sure where that error is sourced from though. Do you know?
return buildRenderResult(root); | ||
} | ||
|
||
export function fireEvent(node: ReactNode, eventName: 'press') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to expose simulating multi-event interactions, they should involve delays between events like touchstart
and touchend
, and hence be async.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one of the hackiest parts of this prototype and will be rewritten completely. Adding delays/async behavior makes sense to me!
import {act} from 'react-test-renderer'; | ||
import ReactFabric from 'react-native/Libraries/Renderer/shims/ReactFabric'; | ||
|
||
type ReactNode = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: is this type representing only host components or would it also represent composite components like ReactTestInstance
did in RTR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently its an incomplete (and maybe inaccurate) typing representing the nodes created by Fabric. We can likely find and import the real typing. Although it may be purposely opaque, having trouble remembering now. But for input, we probably want to use the real typing. For output maybe we create a custom object with a subset of attributes that are useful for testing cases.
|
||
type RootReactNode = $ReadOnlyArray<ReactNode>; | ||
|
||
type RenderResult = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Render result should include the root
view, it it used by RNTL in couple of ways:
- passing it to the users so they can easily make assertions on root element
- using as query start for our own implementation of tree traversal
- check for element being in given element tree using
parent
travels too theroot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out. Adding another follow up item!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to follow up with a simplified PR to get a base landed that we can iterate on
module.exports = { | ||
haste: { | ||
defaultPlatform: 'ios', | ||
platforms: ['android', 'ios', 'native'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a task to follow up with a way to control this
import {act} from 'react-test-renderer'; | ||
import ReactFabric from 'react-native/Libraries/Renderer/shims/ReactFabric'; | ||
|
||
type ReactNode = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will give this a try as a follow up
type ReactNode = { | ||
children: Array<ReactNode>, | ||
props: {...}, | ||
viewName: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
host component name
You can see in the snapshot output, its RCTView or RCTText in this example. I'll have to look back at this with some custom components and see what those will come in as.
memoizedProps
I can't remember at the moment. But I think some properties weren't available as props
but were in memoizedProps
when I was working on this. Will look into it
import {act} from 'react-test-renderer'; | ||
import ReactFabric from 'react-native/Libraries/Renderer/shims/ReactFabric'; | ||
|
||
type ReactNode = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently its an incomplete (and maybe inaccurate) typing representing the nodes created by Fabric. We can likely find and import the real typing. Although it may be purposely opaque, having trouble remembering now. But for input, we probably want to use the real typing. For output maybe we create a custom object with a subset of attributes that are useful for testing cases.
}, | ||
}; | ||
|
||
type RootReactNode = $ReadOnlyArray<ReactNode>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here... this is a manual typing based on the outputs we were inspecting from Fabric. Can be improved or resused from official types probably.
|
||
type RootReactNode = $ReadOnlyArray<ReactNode>; | ||
|
||
type RenderResult = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out. Adding another follow up item!
} | ||
const containerTag = Math.round(Math.random() * 1000000); | ||
act(() => { | ||
ReactFabric.render(element, containerTag, () => {}, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it seems better if that comes for free as part of using RN/Fabric rather than re-implementing one-off errors in the test renderer (if possible). I'm not sure where that error is sourced from though. Do you know?
return buildRenderResult(root); | ||
} | ||
|
||
export function fireEvent(node: ReactNode, eventName: 'press') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one of the hackiest parts of this prototype and will be rewritten completely. Adding delays/async behavior makes sense to me!
@jackpope makes sense. Ping me whenever you need any review or consultation. |
Summary:
Changelog:
Test Plan: